feat(Nav): add icon prop to NavExpandable#12448
Conversation
Align expandable nav sections with NavItem by supporting an optional leading icon. Assisted-by: Cursor
WalkthroughNavExpandable component now accepts an optional ChangesNavExpandable Icon Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview: https://pf-react-pr-12448.surge.sh A11y report: https://pf-react-pr-12448-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-core/src/components/Nav/examples/NavExpandableIcons.tsx (1)
26-88: 💤 Low valueConsider demonstrating toggle behavior more clearly.
Both expandable groups have
isExpandedset totrue. For a better demonstration of the expandable functionality with icons, consider having one group collapsed by default or controlling the expanded state to show the icon in both collapsed and expanded states.📚 Alternative: control expanded state
export const NavExpandableIcons: React.FunctionComponent = () => { const [activeGroup, setActiveGroup] = useState('nav-expandable-icon-group-1'); const [activeItem, setActiveItem] = useState('nav-expandable-icon-group-1_item-1'); + const [expandedGroups, setExpandedGroups] = useState<string[]>(['nav-expandable-icon-group-1']); const onSelect = ( _event: React.FormEvent<HTMLInputElement>, result: { itemId: number | string; groupId: number | string } ) => { setActiveGroup(result.groupId as string); setActiveItem(result.itemId as string); }; const onToggle = ( _event: React.MouseEvent<HTMLButtonElement>, result: { groupId: number | string; isExpanded: boolean } ) => { // eslint-disable-next-line no-console console.log(`Group ${result.groupId} expanded? ${result.isExpanded}`); + setExpandedGroups(prev => + result.isExpanded + ? [...prev, result.groupId as string] + : prev.filter(id => id !== result.groupId) + ); }; return ( <Nav onSelect={onSelect} onToggle={onToggle} aria-label="Expandable with icons global"> <NavList> <NavExpandable title="Expandable Group 1" icon={<CubeIcon />} groupId="nav-expandable-icon-group-1" isActive={activeGroup === 'nav-expandable-icon-group-1'} - isExpanded + isExpanded={expandedGroups.includes('nav-expandable-icon-group-1')} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-core/src/components/Nav/examples/NavExpandableIcons.tsx` around lines 26 - 88, The example always forces both NavExpandable groups open via the isExpanded prop; change it to demonstrate toggle behavior by controlling expansion from state instead. Replace the hardcoded isExpanded on each NavExpandable with a derived value (e.g., isExpanded={activeGroup === 'nav-expandable-icon-group-1'} and isExpanded={activeGroup === 'nav-expandable-icon-group-2'}) or manage a local expandedGroups state and update it in the existing onToggle handler so one group can be collapsed by default and toggles correctly; update NavExpandable usages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-core/src/components/Nav/NavExpandable.tsx`:
- Line 138: The icon span rendered in NavExpandable (the JSX expression {icon &&
<span className={css(styles.navLinkIcon)}>{icon}</span>}) should be marked
decorative by adding aria-hidden="true" to that span; apply the identical change
to the equivalent icon wrapper in NavItem.tsx so both NavExpandable and NavItem
treat the displayed icons as non-accessible/decorative elements.
---
Nitpick comments:
In `@packages/react-core/src/components/Nav/examples/NavExpandableIcons.tsx`:
- Around line 26-88: The example always forces both NavExpandable groups open
via the isExpanded prop; change it to demonstrate toggle behavior by controlling
expansion from state instead. Replace the hardcoded isExpanded on each
NavExpandable with a derived value (e.g., isExpanded={activeGroup ===
'nav-expandable-icon-group-1'} and isExpanded={activeGroup ===
'nav-expandable-icon-group-2'}) or manage a local expandedGroups state and
update it in the existing onToggle handler so one group can be collapsed by
default and toggles correctly; update NavExpandable usages accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a78d09fe-46e4-4718-91f7-d232a555c19f
📒 Files selected for processing (4)
packages/react-core/src/components/Nav/NavExpandable.tsxpackages/react-core/src/components/Nav/__tests__/NavExpandable.test.tsxpackages/react-core/src/components/Nav/examples/Nav.mdpackages/react-core/src/components/Nav/examples/NavExpandableIcons.tsx
| tabIndex={isSidebarOpen ? null : -1} | ||
| {...buttonProps} | ||
| > | ||
| {icon && <span className={css(styles.navLinkIcon)}>{icon}</span>} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check NavItem icon wrapper implementation for aria-hidden
rg -nP -A2 -B2 'navLinkIcon.*icon' --type=tsx --type=ts packages/react-core/src/components/Nav/NavItem.tsxRepository: patternfly/patternfly-react
Length of output: 99
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate NavItem.tsx and inspect icon wrapper + aria-hidden usage
ls -la packages/react-core/src/components/Nav/ || true
if [ -f packages/react-core/src/components/Nav/NavItem.tsx ]; then
rg -n 'navLinkIcon' packages/react-core/src/components/Nav/NavItem.tsx
rg -n 'aria-hidden' packages/react-core/src/components/Nav/NavItem.tsx || true
# show small surrounding area where icon wrapper is rendered
rg -n --context=4 'navLinkIcon' packages/react-core/src/components/Nav/NavItem.tsx
else
echo "NavItem.tsx not found at expected path" >&2
fi
# Also inspect NavExpandable.tsx around the referenced line for consistency
rg -n --context=3 'navLinkIcon' packages/react-core/src/components/Nav/NavExpandable.tsxRepository: patternfly/patternfly-react
Length of output: 1596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/react-core/src/components/Nav/NavItem.tsx"
echo "== aria-hidden matches in NavItem.tsx =="
rg -n 'aria-hidden' "$FILE" || true
echo "== navLinkIcon context in NavItem.tsx =="
rg -n --context=4 'navLinkIcon' "$FILE"Repository: patternfly/patternfly-react
Length of output: 504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/react-core/src/components/Nav/NavExpandable.tsx"
echo "== aria-hidden matches in NavExpandable.tsx =="
rg -n 'aria-hidden' "$FILE" || true
echo "== navLinkIcon context in NavExpandable.tsx =="
rg -n --context=4 'navLinkIcon' "$FILE"Repository: patternfly/patternfly-react
Length of output: 679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '180,240p' packages/react-core/src/components/Nav/NavItem.tsx | cat -n
echo "----"
sed -n '110,170p' packages/react-core/src/components/Nav/NavExpandable.tsx | cat -nRepository: patternfly/patternfly-react
Length of output: 5416
Add aria-hidden="true" to decorative nav icon wrappers in NavExpandable/NavItem
Both packages/react-core/src/components/Nav/NavExpandable.tsx and packages/react-core/src/components/Nav/NavItem.tsx render the icon span without aria-hidden, even though the icon sits next to visible text.
♿ Proposed fix to add aria-hidden
- {icon && <span className={css(styles.navLinkIcon)}>{icon}</span>}
+ {icon && <span className={css(styles.navLinkIcon)} aria-hidden="true">{icon}</span>}Apply the same aria-hidden="true" to the equivalent icon wrapper in NavItem.tsx.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {icon && <span className={css(styles.navLinkIcon)}>{icon}</span>} | |
| {icon && <span className={css(styles.navLinkIcon)} aria-hidden="true">{icon}</span>} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-core/src/components/Nav/NavExpandable.tsx` at line 138, The
icon span rendered in NavExpandable (the JSX expression {icon && <span
className={css(styles.navLinkIcon)}>{icon}</span>}) should be marked decorative
by adding aria-hidden="true" to that span; apply the identical change to the
equivalent icon wrapper in NavItem.tsx so both NavExpandable and NavItem treat
the displayed icons as non-accessible/decorative elements.
|
I was assigned this in Jira. After putting this together, I noticed an existing PR by @logonoff. Will bring to group and see what we want to do with this duplicated work. We may be able to just close this one. |
|
heh
|
|
I'm going to close this one and I'll work on reviewing yours/getting it in @logonoff. |
3 participants
Align expandable nav sections with NavItem by supporting an optional leading icon.
Assisted-by: Cursor
What: Closes #12443
Additional issues:
Summary by CodeRabbit
New Features
Tests
Documentation